-
Notifications
You must be signed in to change notification settings - Fork 14k
fix(span): track unnormalized source len for dep-info #148962
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
compiler/rustc_span/src/lib.rs
Outdated
| self.checksum_hash.encode(s); | ||
| // Do not encode `start_pos` as it's global state for this session. | ||
| self.source_len.encode(s); | ||
| self.original_source_len.encode(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is really needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say if the normalized source len is encoded, then unnormalized source len should also be encoded. I would imagine that if the source file changed, then well, it did materially change.
| ( | ||
| escape_dep_filename(&fmap.name.prefer_local().to_string()), | ||
| fmap.source_len.0 as u64, | ||
| fmap.original_source_len as u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is the goal of the entire PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: can you please add a comment to elaborate here for locality? That is,
source_len -> original_source_len is very subtle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Yeah it is clearer now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert in this part of the compiler, however the changes make sense to me. A few "cosmetic" nits.
| ( | ||
| escape_dep_filename(&fmap.name.prefer_local().to_string()), | ||
| fmap.source_len.0 as u64, | ||
| fmap.original_source_len as u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: can you please add a comment to elaborate here for locality? That is,
source_len -> original_source_len is very subtle.
compiler/rustc_span/src/lib.rs
Outdated
| /// The byte length of this source before normalization. | ||
| pub original_source_len: u32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: I'm not sure how invasive the change is, but we can rename this pair of source lengths (better names welcome):
source_len->normalized_source_lenoriginal_source_len->unnormalized_source_len
When reading this diff, I find that source_len vs original_source_len is really not obvious. I can appreciate if the diffs is intentionally made small to make review easier, but I would prefer if we also do a rename for the source_len field for consistency -- this is really not obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice suggestion. Done in a separate commit.
compiler/rustc_span/src/lib.rs
Outdated
| self.checksum_hash.encode(s); | ||
| // Do not encode `start_pos` as it's global state for this session. | ||
| self.source_len.encode(s); | ||
| self.original_source_len.encode(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say if the normalized source len is encoded, then unnormalized source len should also be encoded. I would imagine that if the source file changed, then well, it did materially change.
|
@rustbot author |
This comment has been minimized.
This comment has been minimized.
|
Fun. clippy is another josh subtree, so it's okay to change r-l/r side since we obviously need to change the compiler tree here. |
|
At least Cargo doesn't have that, otherwise I'll need to do two more PRs 😬 |
|
Oops typo |
This is a preparation for introducing a unnormalized source length field
Add `unnormalized_source_len` field to track the byte length of source files before normalization (the original length). `unnormalized_source_len` is for writing the correct file length to dep-info for `-Zchecksum-hash-algorithm`
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@bors r+ rollup |
fix(span): track unnormalized source len for dep-info Add `unnormalized_source_len` field to track the byte length of source files before normalization (the original length). `unnormalized_source_len` is for writing the correct file length to dep-info for `-Zchecksum-hash-algorithm` Fixes rust-lang#148934
Add
unnormalized_source_lenfield to track the byte lengthof source files before normalization (the original length).
unnormalized_source_lenis for writing the correct file lengthto dep-info for
-Zchecksum-hash-algorithmFixes #148934